set read timeout on client by rymurr · Pull Request #1016 · projectnessie/nessie

rymurr · 2021-03-31T10:18:52Z

This is to address the issue found in apache/iceberg#2328

This change is

This is to address the issue found in apache/iceberg#2328

codecov · 2021-03-31T10:58:33Z

Codecov Report

Merging #1016 (58d7dd1) into main (ac123d3) will increase coverage by 0.07%.
The diff coverage is 70.00%.

@@             Coverage Diff              @@
##               main    #1016      +/-   ##
============================================
+ Coverage     70.53%   70.60%   +0.07%     
  Complexity      122      122              
============================================
  Files           240      241       +1     
  Lines          8955     9003      +48     
  Branches        846      847       +1     
============================================
+ Hits           6316     6357      +41     
- Misses         2202     2210       +8     
+ Partials        437      436       -1

Flag	Coverage Δ	Complexity Δ
java	`69.09% <70.00%> (+0.09%)`	`0.00 <0.00> (ø)`
python	`84.08% <ø> (ø)`	`0.00 <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...ie/client/http/HttpClientReadTimeoutException.java	`25.00% <25.00%> (ø)`	`0.00 <0.00> (?)`
...in/java/org/projectnessie/client/NessieClient.java	`69.04% <42.85%> (-5.96%)`	`0.00 <0.00> (ø)`
...java/org/projectnessie/client/http/HttpClient.java	`75.00% <85.71%> (+1.66%)`	`0.00 <0.00> (ø)`
...ava/org/projectnessie/client/NessieHttpClient.java	`69.69% <100.00%> (+0.46%)`	`0.00 <0.00> (ø)`
...ava/org/projectnessie/client/http/HttpRequest.java	`82.66% <100.00%> (+1.78%)`	`0.00 <0.00> (ø)`
...a/org/projectnessie/versioned/impl/InternalL1.java	`78.78% <0.00%> (ø)`	`0.00% <0.00%> (ø%)`
...g/projectnessie/versioned/impl/InternalBranch.java	`74.89% <0.00%> (+0.41%)`	`0.00% <0.00%> (ø%)`
...ojectnessie/versioned/impl/TieredVersionStore.java	`89.80% <0.00%> (+1.37%)`	`0.00% <0.00%> (ø%)`
...projectnessie/versioned/impl/HistoryRetriever.java	`85.43% <0.00%> (+1.74%)`	`0.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac123d3...58d7dd1. Read the comment docs.

snazy

Reviewable status: 0 of 6 files reviewed, 6 unresolved discussions (waiting on @rymurr)

a discussion (no related file):
Being able to configure network timeouts is a good improvement!
Mind making the connect-timeout configurable as well?

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 68 at r1 (raw file):

    private String password;
    private boolean tracing;
    private int readTimeout = 10;

I'd prefer milliseconds here, just because almost every HTTP client implementation uses milliseconds.

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 68 at r1 (raw file):

    private String password;
    private boolean tracing;
    private int readTimeout = 10;

Just thinking: sun.net.NetworkClient uses the system property sun.net.client.defaultReadTimeout here, maybe we should use the same property as well, but just default to a > 0 default value. WDYT?

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 177 at r1 (raw file):

     * @return {@code this}
     */
    public Builder withReadTimeout(int readTimeout) {

Rename the param to readTimeoutMillis

clients/client/src/main/java/org/projectnessie/client/http/HttpClient.java, line 101 at r1 (raw file):

    private ObjectMapper mapper;
    private SSLContext sslContext;
    private int readTimeout = 10;

I'd maybe give it a bit more by default - maybe 15 or 20 seconds.

clients/client/src/test/java/org/projectnessie/client/http/TestHttpClient.java, line 70 at r1 (raw file):

  @Test
  void testTimeout() {
    HttpHandler handler = h -> {

Looks like this test runs for quite a long time. Guess, this probably needs some Mockito magic.

rymurr

Reviewable status: 0 of 6 files reviewed, 6 unresolved discussions (waiting on @rymurr and @snazy)

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 68 at r1 (raw file):

Previously, snazy (Robert Stupp) wrote…

I'd prefer milliseconds here, just because almost every HTTP client implementation uses milliseconds.

Done.

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 68 at r1 (raw file):

Previously, snazy (Robert Stupp) wrote…

Just thinking: sun.net.NetworkClient uses the system property sun.net.client.defaultReadTimeout here, maybe we should use the same property as well, but just default to a > 0 default value. WDYT?

Done.

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 177 at r1 (raw file):

Previously, snazy (Robert Stupp) wrote…

Rename the param to readTimeoutMillis

Done.

clients/client/src/main/java/org/projectnessie/client/http/HttpClient.java, line 101 at r1 (raw file):

Previously, snazy (Robert Stupp) wrote…

I'd maybe give it a bit more by default - maybe 15 or 20 seconds.

Done.

clients/client/src/test/java/org/projectnessie/client/http/TestHttpClient.java, line 70 at r1 (raw file):

Previously, snazy (Robert Stupp) wrote…

Looks like this test runs for quite a long time. Guess, this probably needs some Mockito magic.

Done.

snazy

Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @rymurr)

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 70 at r2 (raw file):

    private boolean tracing;
    private int readTimeoutMillis = Integer.parseInt(System.getProperty("sun.net.client.defaultReadTimeout", "25000"));
    private int connectionTimeoutMillis = Integer.parseInt(System.getProperty("sun.net.client.defaultConnectionTimeout", "25000"));

That's maybe quite long for a TCP connect-timeout. 3 or 5 seconds is probably good enough.

clients/client/src/main/java/org/projectnessie/client/http/HttpRequest.java, line 149 at r2 (raw file):

    } catch (SocketTimeoutException e) {
      throw new HttpClientReadTimeoutException(
          String.format("Cannot finish request. Timeout while waiting for response with a timeout of %ds", readTimeoutMillis / 1000), e);

The exception message is wrong (both connect + read throw a SocketTimeoutExcepiton), maybe just mention both timeouts (in millis).

clients/client/src/test/java/org/projectnessie/client/http/TestHttpClient.java, line 80 at r2 (raw file):

      try (TestServer server = new TestServer(handler)) {
        get(server.getAddress(), 15000, 1).get().readEntity(ExampleBean.class);
        Assertions.fail();

the Assertions.fail() is superfluous here ;)

rymurr

Reviewable status: 0 of 6 files reviewed, 4 unresolved discussions (waiting on @rymurr and @snazy)

clients/client/src/main/java/org/projectnessie/client/NessieClient.java, line 70 at r2 (raw file):

Previously, snazy (Robert Stupp) wrote…

That's maybe quite long for a TCP connect-timeout. 3 or 5 seconds is probably good enough.

Done.

clients/client/src/main/java/org/projectnessie/client/http/HttpRequest.java, line 149 at r2 (raw file):

Previously, snazy (Robert Stupp) wrote…

The exception message is wrong (both connect + read throw a SocketTimeoutExcepiton), maybe just mention both timeouts (in millis).

Where did you see that? I saw a different exception for connect. ConnectException or something.

clients/client/src/test/java/org/projectnessie/client/http/TestHttpClient.java, line 80 at r2 (raw file):

Previously, snazy (Robert Stupp) wrote…

the Assertions.fail() is superfluous here ;)

yup, removed.

snazy

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @rymurr)

clients/client/src/main/java/org/projectnessie/client/http/HttpRequest.java, line 149 at r2 (raw file):

Previously, rymurr (Ryan Murray) wrote…

Where did you see that? I saw a different exception for connect. ConnectException or something.

In the javadoc for java.net.Socket#connect(java.net.SocketAddress, int). But since you mention it, IIRC it's really ConnectException - interesting, that it's different from the javadoc.

rymurr requested a review from snazy March 31, 2021 10:18

rymurr mentioned this pull request Mar 31, 2021

Ensure the metadata isn't cleaned up in the case of unknown error apache/iceberg#2324

Closed

set read timeout on client

419c322

This is to address the issue found in apache/iceberg#2328

rymurr force-pushed the set-read-timeout branch from c5c6536 to 419c322 Compare March 31, 2021 10:33

fix checkstyle

2bb09d7

snazy requested changes Mar 31, 2021

View reviewed changes

code review

ccb872b

rymurr commented Apr 1, 2021

View reviewed changes

snazy requested changes Apr 1, 2021

View reviewed changes

rymurr commented Apr 1, 2021

View reviewed changes

code review pt 2

58d7dd1

snazy approved these changes Apr 1, 2021

View reviewed changes

rymurr merged commit 2c71a82 into projectnessie:main Apr 1, 2021

rymurr deleted the set-read-timeout branch April 1, 2021 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set read timeout on client#1016

set read timeout on client#1016
rymurr merged 4 commits intoprojectnessie:mainfrom
rymurr:set-read-timeout

rymurr commented Mar 31, 2021 •

edited by jacques-n

Loading

Uh oh!

codecov bot commented Mar 31, 2021 •

edited

Loading

Uh oh!

snazy left a comment

Uh oh!

rymurr left a comment

Uh oh!

snazy left a comment

Uh oh!

rymurr left a comment

Uh oh!

snazy left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rymurr commented Mar 31, 2021 • edited by jacques-n Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

rymurr left a comment

Choose a reason for hiding this comment

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

rymurr left a comment

Choose a reason for hiding this comment

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rymurr commented Mar 31, 2021 •

edited by jacques-n

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading